Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utop: warning when modules collide with utop deps (will become error in 5.1) #8735

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jchavarri
Copy link
Collaborator

It seems dune loads the dependencies of utop when running dune utop, which can lead to conflics for users trying to run utop sessions in projects with modules that can collision with utop deps.

In 4.14, this situation triggered a warning, like this PR shows. But in 5.1 it becomes a hard error. From OCaml compiler changelog:

  • #11635, #5461, #10564: turn warning 31 (Module_linked_twice) into a hard error
    for ocamlc — this was already an error with ocamlopt.
    (Hugo Heuzard, review by Valentin Gatien-Baron and Gabriel Scherer)

@Alizter
Copy link
Collaborator

Alizter commented Sep 22, 2023

@rgrinberg I asked you about this issue in person but I couldn't remember the solution you had nor could I find anything on it. Could you mention it here?

FTR @jchavarri IIRC there is a mode for utop where it doesn't load the libraries that were used to compile it, that you have to use. I think its pretty peculiar that this is even a thing. Can't remember how to switch the mode however.

@rgrinberg
Copy link
Member

I was talking about expunging. I don't think it would help here.

I'm not sure what's the solution nor am I even certain this is a dune problem. I don't know of a way to produce a utop toplevel without linking its dependencies.

@nojb any ideas perhaps?

@nojb
Copy link
Collaborator

nojb commented Sep 25, 2023

dune utop works fine if the code being loaded into the toplevel depends on one or more of the libraries utop itself links against (eg lwt, react). On the other hand, it cannot link modules that have a name identical to one the modules that are already linked into it (eg React, Lwt, etc). This is a basic limitation of the OCaml toolchain.

@jchavarri is it right that in your example in ocaml-community/utop#463, the react library is being vendored in server-reason-react? That would explain the name clash.

In the toplevel, there is a way to remove module names from the global scope (expunging). utop (like the standard toplevel) already expunges compiler-libs, and one could imagine expunging more names (eg React in this case), which would technically allow you to link modules with those names. However, note that expunging means that utop will not recognize that the React module being linked is the same as the (expunged) React module that it linked with originally, and that may lead to some puzzling behaviours.

In any case, note that dune utop does not expunge the custom utop toplevel at all (we don't want to link against compiler-libs).

In summary:

  • If you are vendoring react, then it will clash with the react that is linked into utop (this has nothing to do with Dune). The simplest way to fix this problem is not to vendor react, but link with it "normally".
  • You may experiment with expunging more things out of utop, but results may be unpredictable.
  • Dune does not do any expunging when building the custom utop executable, so it is unlikely that it will be possible to address this issue at the level of Dune. But, if you solve it at the level of utop you may still leverage Dune by using #use_output "dune ocaml top".

@jchavarri
Copy link
Collaborator Author

Thanks for the thorough response @nojb !

@jchavarri is it right that in your example in ocaml-community/utop#463, the react library is being vendored in server-reason-react? That would explain the name clash.

This issue was opened by @davesnx who is working on server-reason-react. This library does not vendor the OCaml react library. Rather, this package provides reimplementations in OCaml of the Melange bindings defined in reason-react. The goal is to compile the same code to either the client / JS (with Melange and reason-react) or the server (OCaml and server-reason-react), so the native implementation has to follow what the Melange one is doing, which in this case means naming the library react.

So the server-reason-react code is fully unrelated to the react library used by utop. It's just a case of name collision between unrelated libraries.

In the toplevel, there is a way to remove module names from the global scope (expunging). utop (like the standard toplevel) already expunges compiler-libs, and one could imagine expunging more names (eg React in this case), which would technically allow you to link modules with those names. However, note that expunging means that utop will not recognize that the React module being linked is the same as the (expunged) React module that it linked with originally, and that may lead to some puzzling behaviours.

This sounds exactly like what we need. And I understand that because the server-reason-react is its own thing, the puzzling behavior mentioned would not apply in our case?

I found some related code in the utop repo, I understand the suggested approach would be to add React to this list?

  • Dune does not do any expunging when building the custom utop executable,

Why doesn't Dune do any expunging? I understood expunging prevents some issues with compiler-libs and other libraries, so it looked useful to have it in all cases.

@nojb
Copy link
Collaborator

nojb commented Sep 25, 2023

I found some related code in the utop repo, I understand the suggested approach would be to add React to this list?

Yes. But to be clear, I am not recommending expunging as a solution (it is a hack and may have unexpected consequences). There may be other ways to work around the issue. For example, you could have a compilation unit called Server_react that does module React = struct ... end inside and use -open Server_react when compiling on the server. Or simply doing module React = Server_react in the source code somehow.

Why doesn't Dune do any expunging? I understood expunging prevents some issues with compiler-libs and other libraries, so it looked useful to have it in all cases.

I guess no one complained about it before. One downside of expunging is that it is a hack, and can sometimes produce unexpected results. For example utop has special support for lwt (it resolves top-level values of type 'a Lwt.t). I suspect that this will stop working if Lwt is expunged (but haven't tried it).

@jchavarri
Copy link
Collaborator Author

For example, you could have a compilation unit called Server_react that does module React = struct ... end inside and use -open Server_react when compiling on the server. Or simply doing module React = Server_react in the source code somehow.

@davesnx is something like this possible?

@davesnx
Copy link
Contributor

davesnx commented Sep 27, 2023

It's possible, but I wonder if changing the interface of the package is a good solution to have it working just with utop.

I can suggest to vendor React (and any other dependency) and rename it to a hard-to-collide name, maybe it's a proper solution but I raised the issue in order to have more ideas on how to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants